Skip to content

Conversation

@alamb
Copy link
Contributor

@alamb alamb commented Apr 23, 2023

Which issue does this PR close?

Related to #6066

Rationale for this change

While reviewing #6093 from @RTEnzyme I noticed there were kernels in arrow-rs for some of the bitwise operations: https://docs.rs/arrow/latest/arrow/compute/kernels/bitwise/index.html

What changes are included in this PR?

Use the kernels from arrow-rs rather than redefining them in datafusion

Are these changes tested?

Covered by existing tests

Are there any user-facing changes?

No

@github-actions github-actions bot added the physical-expr Changes to the physical-expr crates label Apr 23, 2023
let right =
Arc::new(Int32Array::from(vec![Some(1), Some(3), Some(7)])) as ArrayRef;
let mut result = bitwise_and(left.clone(), right.clone())?;
let mut result = bitwise_and_dyn(left.clone(), right.clone())?;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed these kernels to match the naming scheme in arrow-rs (where _dyn is used to refer to kernels that take ArrayRef

create_dyn_kernel!(bitwise_xor_dyn, bitwise_xor);
create_dyn_kernel!(bitwise_and_dyn, bitwise_and);

pub(crate) fn bitwise_shift_right_dyn(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly there are no shift_right or shift_left in arrows

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also found the upstream tracking ticket in arrow -- apache/arrow-rs#2741 and added links in the code here

@alamb alamb merged commit fe20eee into apache:main Apr 24, 2023
@alamb alamb deleted the alamb/simplify_binary branch April 24, 2023 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-expr Changes to the physical-expr crates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants